certman: enforce leaf is end-entity in VerifyCerts_buffer#1075
certman: enforce leaf is end-entity in VerifyCerts_buffer#1075yosuke-wolfssl wants to merge 1 commit into
Conversation
wolfSSL-Fenrir-bot
left a comment
There was a problem hiding this comment.
Fenrir Automated Review — PR #1075
Scan targets checked: wolfssh-bugs, wolfssh-src
No new issues found in the changed files. ✅
aidangarske
left a comment
There was a problem hiding this comment.
🐺 Skoll Code Review
Overall recommendation: COMMENT
Findings: 2 total — 2 posted, 1 skipped
Posted findings
- [Low] Leaf certificate is now fully re-parsed on every verification, including non-FPKI builds —
src/certman.c:404-424 - [Info] Added multi-line explanatory comment blocks —
src/certman.c:387-389,242-244,480-481
Skipped findings
- [Info] Added multi-line explanatory comment blocks
Review generated by Skoll.
|
|
||
| if (ret == 0) { | ||
| WLOG(WS_LOG_CERTMAN, "certificate didn't match profile"); | ||
| if (ret == WS_SUCCESS) { |
There was a problem hiding this comment.
🔵 [Low] Leaf certificate is now fully re-parsed on every verification, including non-FPKI builds
💡 SUGGEST question
Previously the leaf decode (wc_InitDecodedCert/wc_ParseCert) only happened inside #ifndef WOLFSSH_NO_FPKI. The fix correctly makes the CA-leaf rejection unconditional, but as a side effect every successful verification now performs an additional full wc_ParseCert of certLoc[0] (on top of the parse already done inside wolfSSL_CertManagerVerifyBuffer). This is the intended security tradeoff and the cost is small, but it is worth confirming it is acceptable on constrained targets. If wc_ParseCert were ever to fail for a leaf that already passed CertManagerVerifyBuffer, a previously-successful (non-FPKI) verification would now return WS_CERT_OTHER_E. In practice CertManagerVerifyBuffer already parses the same buffer, so this is very unlikely, but it is a behavioral change for the non-FPKI path.
Recommendation: Confirm the extra leaf parse per verification is acceptable for the target platforms. No code change required — the security benefit (unconditional CA-leaf rejection) justifies the cost.
certman: enforce leaf is an end-entity certificate in VerifyCerts_buffer
Problem
wolfSSH_CERTMAN_VerifyCerts_bufferdid not enforce that the leaf(
certLoc[0]) is an end-entity certificate. The only leafisCAcheck livedinside
CheckProfile, which is gated by#ifndef WOLFSSH_NO_FPKI:In a
WOLFSSH_CERTS && WOLFSSH_NO_FPKIbuild, that block is absent, so anattacker holding any CA certificate in the deployed trust chain (plus its
private key) could present that CA as the leaf. It passes
wolfSSL_CertManagerVerifyBuffersignature validation and the function returnsWS_SUCCESS— a certificate-based authentication bypass (CWE-295).The wolfsshd-level mitigation (fail-closed when no
AuthorizedKeysFileis set)was already in place from earlier commits, but the bypass existed at the
library API, which is reachable by any consumer of
WOLFSSH_CERTMAN.Addressed by f_5899.
Fix
src/certman.c:Decode the leaf once and reject it unconditionally when it asserts CA
basic constraints, regardless of FPKI:
Under FPKI, the same decoded cert additionally must match a profile
(
CheckProfile), via anelse if. The leaf is parsed withcm->cmso thesigner (
ca) is resolved forCheckProfile's issuer-DN match.Removed the now-redundant
!cert->isCAcheck fromCheckProfile(the callerenforces it unconditionally before
CheckProfileruns).Converted both
DecodedCertusages (the leaf block andCertManIntermediateIsCA) to theWOLFSSH_SMALL_STACKheap-allocationpattern, with a distinct log on the allocation-failure (fail-closed) path.
Tests
tests/api.c(test_wolfSSH_CertMan, guarded#ifndef WOLFSSH_NO_ECDSA):WS_CERT_PROFILE_E. Runs in both FPKI-enabled and non-FPKI builds.the CA still verifies with
WS_SUCCESS.certman_make_chain()helper for the length-prefixed chainframing to avoid duplicated byte-shift encoding.
Verification
CA-as-leaf case returns
WS_SUCCESSand the test aborts; with the fix itreturns
WS_CERT_PROFILE_E.api.testpasses under:WOLFSSH_NO_FPKI(default + small-stack), and thedefault FPKI-active build.
src/certman.ccompiles warning-free with-Wall -Wextrain bothsmall-stack and default-stack configurations.